Added option to use datetime stamp during logging. (IDFGH-3855) #5761
Conversation
Changed log timestamp formatting to use unsigned int, allowing for smaller buffer.
|
|
github-actions
bot
changed the title
|
Thanks for your contribution. |
There was a problem hiding this comment.
Hi @Molorius ,
Thanks for sending this. I have a couple of minor comments but the change looks very useful to me, look forward to integrating it.
components/log/Kconfig
Outdated
| @@ -70,4 +70,12 @@ menu "Log output" | |||
| bool "System Time" | |||
| endchoice | |||
|
|
|||
| config LOG_TIMESTAMP_SOURCE_SYSTEM_DATETIME | |||
There was a problem hiding this comment.
Relatively minor, but for ease of use suggest adding this as a "choice" so the choices become "Milliseconds Since Boot", "System Time", "System Date and Time"
(Means some of the macro guards in esp_log.h will need to be updated as well.)
There was a problem hiding this comment.
It is now another choice in the kconfig.
components/log/log_freertos.c
Outdated
| @@ -52,7 +52,11 @@ void esp_log_impl_unlock(void) | |||
|
|
|||
| char *esp_log_system_timestamp(void) | |||
There was a problem hiding this comment.
The description of this function in esp_log.h needs to be updated to match
There was a problem hiding this comment.
Would you prefer changing the name of the function or splitting it into esp_log_system_timestamp and esp_log_system_datetimestamp? The xTaskGetSchedulerState portion would be put into a static function for use between both.
There was a problem hiding this comment.
@Molorius Good point! That is probably a cleaner solution, thank you for pointing it out.
However if it becomes less clean to implement in some way, I'm OK with the current implementation - it just needs to be noted in the header that it might return a date+time depending on config.
There was a problem hiding this comment.
I separated the datetime elements into a separate function and updated esp_log.h to match.
Added better comments for datetimestamp function. Have datetimestamp as a full choice in kconfig rather than an additional option.
|
Hi @Molorius , Thanks for being patient while I got back to you. This looks great to me, I've squashed the commits and pushed it into our internal review & merge queue. The PR will be updated once it has merged. Angus |
|
As it happens, at the last minute we've decided to try and implement this in a different way. Rather than having to add a whole new function and kconfig item for each new format, we're going to try to find an easier way to customize the log timestamp. You don't need to do anything, this PR will still be updated once this functionality is available. |
espressif-bot
added
Status: Done
Added option to use datetime stamp during logging.
Also changed log timestamp formatting to use unsigned int, allowing for smaller buffer.